Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: inscriptions settings btn and available balance #6050

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

alter-eggo
Copy link
Collaborator

@alter-eggo alter-eggo commented Dec 27, 2024

Try out Leather build 7f838c2Extension build, Test report, Storybook, Chromatic

this pr fixes:

  • available balance tooltip text
  • disable swaps on testnet
  • updates query package to prevent infinite brc20 request, related pr
  • inscription settings btn styles:

Screenshot 2024-12-27 at 18 57 12

@fbwoolf fbwoolf self-requested a review December 27, 2024 15:51
@fbwoolf
Copy link
Contributor

fbwoolf commented Dec 27, 2024

I initially approved this, but I'm not confident adding locked in the tooltip is correct without actually deducting it from the account card available balance?

@fbwoolf fbwoolf changed the title Fix/inscriptions settings btn fix: inscriptions settings btn Dec 27, 2024
@fbwoolf fbwoolf changed the title fix: inscriptions settings btn fix: inscriptions settings btn and available balance Dec 27, 2024
@fbwoolf
Copy link
Contributor

fbwoolf commented Dec 27, 2024

Bitflow isn't initializing for me on this branch, but it might be a local issue. Looking into it...

EDIT: Nvm, this was me and my .env file.

@markmhendrickson markmhendrickson self-requested a review December 30, 2024 12:42
@markmhendrickson
Copy link
Collaborator

markmhendrickson commented Dec 31, 2024

🚧 Undergoing QA

I'll test the following cases manually. Can we also add automated tests for them, even if warranted in separate PR(s) to unblock release of this one?

Ideally we'd have automated tests that check on the UI level in addition to the function one, to ensure that updated values appear immediately once their related events occur (i.e. to ensure that caching doesn't incur delays to updating their display as expected by users).

🤔 == cases in which I'm not sure how to test without configuring the dev wallet with special assets
✅ == cases that have succeeded in testing
🚫 == cases that have failed in testing

Home view – total balance

  • Total balance on home view does not update due to pending outbound BTC transfer 🚫
  • Total balance on home view does not update due to pending outbound STX transfer ✅
  • Total balance on home view does not update due to pending outbound SIP-10 transfer
  • Total balance on home view does not update due to pending inbound BTC transfer
  • Total balance on home view does not update due to pending inbound STX transfer
  • Total balance on home view does not update due to pending inbound SIP-10 transfer

Home view - available balance

  • Available balance on home view does update due to pending outbound BTC transfer
  • Available balance on home view does update due to pending outbound STX transfer
  • Available balance on home view does update due to pending outbound SIP-10 transfer
  • Available balance on home view does update due to locked STX amount
  • Available balance on home view does update due to uneconomical UTXOs 🤔
  • Available balance on home view does update due to unprotected collectibles 🤔

Home view – token balances

  • BTC balance on home view does not update due to pending outbound BTC transfer
  • STX balance on home view does not update due to pending outbound STX transfer
  • SIP-10 balance on home view does not update due to pending outbound SIP-10 transfer

Send form view

  • Available balance on send form view for BTC does update due to pending outbound BTC transfer
  • Available balance on send form view for STX does update due to pending outbound STX transfer
  • Available balance on send form view for SIP-10 does update due to pending outbound SIP-10 transfer
  • Available balance on send view for STX does update due to locked STX amount
  • Available balance on send view for BTC does update due to uneconomical UTXOs 🤔
  • Available balance on send view for BTC does update due to unprotected collectibles 🤔

Swap form view

  • Available balance on swap form view for BTC->sBTC does update due to pending outbound BTC transfer
  • Available balance on swap form view for STX->SIP-10 does update due to pending outbound STX transfer
  • Available balance on swap form view for SIP-10->STX does update due to pending outbound SIP-10 transfer
  • Available balance on swap view for STX->SIP-10 does update due to locked STX amount

Select account / getAddresses view

  • Total balance for account to select does not update due to pending outbound BTC transfer
  • Total balance for account to select does not update due to pending outbound STX transfer
  • Total balance for account to select does not update due to pending outbound SIP-10 transfer
  • Total balance for account to select does not update due to pending inbound BTC transfer
  • Total balance for account to select does not update due to pending inbound STX transfer
  • Total balance for account to select does not update due to pending inbound SIP-10 transfer

API views

  • Available balance on approval view for sendTransfer does update due to pending outbound BTC transfer
  • Available balance on approval view for sendTransfer does update due to uneconomical UTXOs 🤔
  • Available balance on approval view for sendTransfer does update due to unprotected collectibles 🤔
  • Available balance on approval view for signPsbt does update due to pending outbound BTC transfer
  • Available balance on approval view for signPsbt does update due to uneconomical UTXOs 🤔
  • Available balance on approval view for signPsbt does update due to unprotected collectibles 🤔
  • Available balance on approval view for signMessage does update due to pending outbound BTC transfer
  • Available balance on approval view for signMessage does update due to uneconomical UTXOs 🤔
  • Available balance on approval view for signMessage does update due to unprotected collectibles 🤔
  • Available balance on approval view for stx_signTransaction does update due to pending outbound STX transfer
  • Available balance on approval view for stx_signTransaction does update due to locked STX amount
  • Available balance on approval view for stx_signMessage does update due to pending outbound STX transfer
  • Available balance on approval view for stx_signMessage does update due to locked STX amount

@kyranjamie
Copy link
Collaborator

This feature, and all the bugs/inaccuracies it introduced are the result of fast-paced work building on top an already old/poorly-implemented feature.

We need to rethink how we manage UTXOs, and in turn calculate balances, from ground up. This was already in-flight prior to the sBTC rush. It may well be better to focus on that than first fixing these issues?

I think @alexp3y and I should return to this next week, focusing on:

  1. Finalising UTXO fetching worker, exposed via UtxoService, and implementing this in extension
  2. Implementing and testing a BalanceService that aggregates data from necessary sources (Address UTXOs, Pending Mempool UTXOs etc) to derive accurate balances

@fbwoolf
Copy link
Contributor

fbwoolf commented Jan 3, 2025

Agree here, maybe we can merge this with manually testing and then address the integration tests with the larger service refactor work?

@markmhendrickson
Copy link
Collaborator

Note that I intend to complete manual QA of this PR on Monday, and I've already surfaced one case that doesn't seem to work in this PR atm ("Total balance on home view does not update due to pending outbound BTC transfer", in that in my testing, it does update undesirably).

Other folks are welcome to contribute to this manual QA in the meantime if they want to go through my list and check things off. cc @DeeList @314159265359879

I'm also in support of the larger code changes (that will help mobile as well presumably). @kyranjamie is your sense that these larger efforts will be needed to get this PR finalized, or might we be able to do them in parallel and patch this PR separately in the meantime?

@fbwoolf
Copy link
Contributor

fbwoolf commented Jan 3, 2025

Yep, it looks like in code we filter out pending utxos for both total and available BTC balance, so that needs to be changed before this is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants